Skip tests that require kerchunk #259
Conversation
TomNicholas
added
Kerchunk
TomNicholas
temporarily deployed
to
test-release
GitHub Actions
Inactive
TomNicholas
temporarily deployed
to
test-release
GitHub Actions
Inactive
TomNicholas
temporarily deployed
to
test-release
GitHub Actions
Inactive
| @@ -0,0 +1,60 @@ | |||
| name: min-deps | |||
There was a problem hiding this comment.
There was probably a better way to do this than just making an entire extra workflow file but my github-actions-fu is weak
| "xarray>=2024.06.0", | ||
| "kerchunk>=0.2.5", | ||
| "h5netcdf", | ||
| "numpy>=2.0.0", | ||
| "ujson", | ||
| "packaging", | ||
| "universal-pathlib", | ||
| "numcodecs", |
There was a problem hiding this comment.
The minimum set of dependencies should look like this, and you could probably even make universal-pathlib optional for reading from non-remote files...
There was a problem hiding this comment.
Actually I re-added ujson, because we need it for parsing existing kerchunk-formatted references.
There was a problem hiding this comment.
Let's also leave the question of whether we really need fsspec/universal_pathlib for later, and just concentrate on removing kerchunk.
There was a problem hiding this comment.
It might be nice to have more splits in the optional deps depending on the usecase, kind of like how Xarray handles it. We could have splits for different readers, writers and local/cloud usecases?
There was a problem hiding this comment.
I was thinking the same, but kept this PR targeted for now.
TomNicholas
temporarily deployed
to
test-release
GitHub Actions
Inactive
TomNicholas
temporarily deployed
to
test-release
GitHub Actions
Inactive
There was a problem hiding this comment.
Probably for another discussion, but could we have all deps and optional deps in pyproject.toml then the optional h5py in a conda env?
Or maybe it's a good reason to transition to Pixi 🥇
There was a problem hiding this comment.
I know nothing about pixi, but would be open to that.
There was a problem hiding this comment.
@maxrjones or @cisaacstern might have some real experience with it, but one really cool seeming thing is that you can have both conda and pip deps all in a pyproject.toml
| "xarray>=2024.06.0", | ||
| "kerchunk>=0.2.5", | ||
| "h5netcdf", | ||
| "numpy>=2.0.0", | ||
| "ujson", | ||
| "packaging", | ||
| "universal-pathlib", | ||
| "numcodecs", |
There was a problem hiding this comment.
It might be nice to have more splits in the optional deps depending on the usecase, kind of like how Xarray handles it. We could have splits for different readers, writers and local/cloud usecases?
| @@ -13,9 +12,17 @@ | |||
| from virtualizarr.backend import FileType | |||
| from virtualizarr.manifests import ManifestArray | |||
| from virtualizarr.readers.kerchunk import _automatically_determine_filetype | |||
| from virtualizarr.tests import has_astropy, has_tifffile, network, requires_s3fs | |||
| from virtualizarr.tests import ( | |||
There was a problem hiding this comment.
This is a great addition! Thanks for implementing it.
There was a problem hiding this comment.
Another good idea just stolen from xarray 😅
There was a problem hiding this comment.
The source of all good code
All I've done here is skip all of those that currently require kerchunk, and add a new CI job to check that the others pass without kerchunk installed. I'm hoping that we can merge this PR then incrementally re-write all of the tests (apart from those in
test_readers/test_kerchunk.py&test_writers/test_kerchunk.py) to run and test the same functionality overall without requiring kerchunk.